Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(AchievementsRelationManager): gracefully handle bulk re-inserts of trashed credit #3173

Merged

Conversation

wescopeland
Copy link
Member

Resolves https://discord.com/channels/476211979464343552/1026595325038833725/1335738485273002056.

Currently, a server error is thrown when trying to re-insert a soft deleted credit record using this tool:
Screenshot 2025-02-03 at 4 48 05 PM

Steps to repro the server error:

  1. Visit http://localhost:64000/manage/games/1.
  2. Select every achievement on the list, bulk add some kind of credit for them.
  3. Now, navigate to the achievement and delete that credit record.
  4. Navigate back to http://localhost:64000/manage/games/1, and with the bulk tool attempt to reinsert the same type of credit for the same user.

Now, similar to AuthorshipCreditsRelationManager, if we're reinserting a trashed record, we'll restore it.

@wescopeland wescopeland requested a review from a team February 3, 2025 21:49
@Jamiras
Copy link
Member

Jamiras commented Feb 3, 2025

Now, navigate to the achievement and delete that credit record.

I can't figure out how to do that. I can edit the credit record, but there's no delete option.

@wescopeland
Copy link
Member Author

Interesting! Here is what I see:

Brave.Browser.mp4

Could be perms-related? AchievementAuthorPolicy::delete():

public function delete(User $user, AchievementAuthor $achievementAuthor): bool
{
    return $user->hasAnyRole([
        Role::MODERATOR,
        Role::TEAM_ACCOUNT,
    ]);
}

@Jamiras
Copy link
Member

Jamiras commented Feb 3, 2025

Yep. I didn't think to try "Moderator". I tried "Developer", "Game Editor" and "Admin".

Copy link
Member

@Jamiras Jamiras left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unrelated: Noticed that trying to add a user with a custom display_name by their display_name doesn't work. Entering their username shows the display_name in the list, and then that can be selected.

@wescopeland
Copy link
Member Author

wescopeland commented Feb 5, 2025

Strange, I am able to reproduce that issue for either the relation manager or the bulk add tool:

Brave.Browser.mp4

Possible I am doing something wrong? Going to go ahead and merge this, but happy to hunt down a bug in a subsequent PR.

@wescopeland wescopeland merged commit 63473a1 into RetroAchievements:master Feb 5, 2025
9 checks passed
@wescopeland wescopeland deleted the credit-trashed-server-error branch February 5, 2025 22:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants